fix(downloader): distinguish read from write failures and retry transient ones#10985
Merged
Conversation
…ient ones Two independent defects in the download path, both surfaced by the same incident (#10982). A failed `io.Copy` was always reported as "failed to write file", because `io.Copy` folds read and write errors into a single return value. A peer-cancelled HTTP/2 stream therefore presented as a filesystem failure and sent an investigation after mount permissions while the disk was healthy. The source is now wrapped in a recorder so the error names the side that actually broke, and a write failure names the `.partial` it was writing rather than the final blob path. The plan runner returned on the first task error with no retry, so one transient stream cancel discarded every file already downloaded in a multi-file materialization. The `.partial` resume machinery already existed but was unreachable because nothing made a second attempt. Transient failures (dropped transport, mid-stream read failure, stall, 5xx, 429) are now retried with bounded exponential backoff and resume from the partial; permanent ones (4xx, checksum mismatch, local write failure, caller cancellation) fail immediately. Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Assisted-by: Claude:claude-opus-4-8 [Claude Code]
mudler
added a commit
that referenced
this pull request
Jul 21, 2026
…#11017) backendLoader logged "BackendLoader starting" at INFO as its very first statement, unconditionally. That reads as "a model is being loaded", but backendLoader is not only a load path: in distributed mode Load() deliberately bypasses the local cache and calls backendLoader on every inference request so SmartRouter can re-pick a replica per request. The model is already resident, no process is spawned, and nothing is loaded, yet the banner fires at request rate. On a live cluster this produced ~5 "BackendLoader starting" lines per second for a single embedding model, sustained, starting 22 seconds after the load had already completed. The model was state=loaded with in_flight=0 and exactly one backend process on the worker. It looked exactly like a retry storm and cost real debugging time during an unrelated production investigation. The adjacent "effective runtime tuning" banner, documented as "logged once per load", had the same problem for the same reason. Emit both banners at INFO only when the model is not already resident, and keep the per-call trace at DEBUG for anyone following the routing path. isResident is a plain store lookup with no health probe and no eviction, so it is safe on the per-request hot path (unlike checkIsLoaded, which probes and can evict). Same class of defect as #10985: a log line that sends the reader after the wrong thing. Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash] Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10982.
1. A read error was reported as a write failure
io.Copyreturns read and write errors indistinguishably, anduri.golabelled every one of themfailed to write file.In the incident that surfaced this,
stream error: stream ID 23; CANCEL; received from peer— anhttp2.StreamError, which can only come fromsource.Read— was reported as a write failure. That sent the investigation after filesystem permissions and CIFS write semantics for an hour. The disk was fine the whole time.The copy source is now wrapped in a small recorder that remembers the last non-EOF read error; comparing it against the error
io.Copyreturned says which side broke. A read failure names the URL it was reading and the file it was writing into; a write failure now names the.partial, which is the file actually being written, rather than the final blob path.2. One transient error aborted a whole multi-file materialization
DownloadFilesWithContextreturned on the first task error with no retry. Stream ID 23 means roughly the twelfth request on that connection: eleven files had already downloaded successfully, and all of it was discarded.The
.partialresume machinery already existed (uri.gocomputesstartPosfrom the existing partial's size) but nothing ever retried, so it was unreachable in practice.Transient failures are now retried within the plan, resuming from the partial.
downloadClient.Do.partialErrUserCancelledIsRetryableshort-circuits onctx.Err() != nilbefore classificationBounded at 3 attempts with 2s/4s backoff, tunable via
DownloadRetryAttempts/DownloadRetryBaseDelayin the style of the existingDownloadStallTimeout. The small budget is deliberate: resume makes a retry cheap in bytes, but a tight loop against a genuinely broken remote on multi-GB files is its own hazard. Backoff waits onctx.Done().No existing retry helper was found in
pkg/orcore/, so this is new.Scope
Both fixes are storage-backend independent and reproduce on local disk with a flaky network — no CIFS and no concurrency required. They were surfaced by #10981 but are not caused by it.
Plan callers (
pkg/modelartifacts/materializer.go,core/gallery/models.go) inherit the retry with no signature change and are untouched.Repo-wide grep including
tests/e2e/found nothing pinningfailed to write file,invalid status code,resume request for, or plan-abort behaviour.Tests
Ginkgo specs in
pkg/downloader/retry_test.go, driven by a flaky range server that aborts the connection mid-body:.partialbytes=N-Range headers — proving resume rather than restart — with correct final contentRed before implementation:
That first failure message is the bug verbatim: a mid-stream read abort labelled as a write failure.
Verification:
go build ./core/... ./pkg/...clean;go test ./pkg/downloader/ ./pkg/xio/ok;make lint0 issues.🤖 Generated with Claude Code